feat(search): add max_snippet_lines to cap returned snippets (semble#198) - #80
Merged
Conversation
…198)
Port semble#198's max_snippet_lines: results can return a preview of each
chunk instead of the full content, so an agent spends fewer tokens
confirming a location before navigating to the file.
Semantics (utils::format_results / result_to_dict):
- None → full chunk content
- 0 → omit `content` (file path + line range only)
- N > 0 → first N lines
Also flattens the wire dict to match upstream after #198: results are now
`{file_path, start_line, end_line, score, content?}` at the top level
(dropping the nested `chunk` wrapper, `location`, and `language`). This
follows the upstream shape csp had faithfully mirrored before #198
reshaped it; the library `SearchResult` is unchanged.
Surface:
- CLI `search` / `find-related`: `--max-snippet-lines N`, default None
(full content — human-facing).
- MCP `search` / `find_related`: `max_snippet_lines` param, default 10
(token-frugal preview — agent-facing). Absent → 10, JSON null → full,
0 → location only (tri-state via serde field default).
Not in scope: savings accounting (semble#206). csp does not yet wire
save_search_stats into the search flow (no file_sizes on CspIndex), so
there is nothing to correct until savings telemetry is wired — tracked
separately.
Refs #75
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 10 |
| Duplication | 8 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
No issues found across 6 files
Architecture diagram
sequenceDiagram
participant CLI as CLI User (csp search)
participant MCP as MCP Client (agent)
participant Handler as Search Handler (CLI / MCP)
participant Utils as format_results / result_to_dict
participant Search as IndexCache / Search Engine
Note over CLI,Search: NEW: max_snippet_lines parameter & flat wire shape
CLI->>Handler: csp search --max-snippet-lines N "query" ./repo (default: None)
MCP->>Handler: JSON-RPC search(query, repo, max_snippet_lines=10|null) (default: 10)
Handler->>Search: search(query, repo, top_k)
Search-->>Handler: Vec<SearchResult>
Handler->>Utils: format_results(query, results, max_snippet_lines)
loop per result
Utils->>Utils: Build flat dict: file_path, start_line, end_line, score
alt max_snippet_lines = None
Utils->>Utils: include full content
else max_snippet_lines = 0
Utils->>Utils: omit content
else max_snippet_lines = N
Utils->>Utils: include first N lines as content
end
end
Note over Utils: CHANGED: no nested "chunk"/"location"/"language" fields
Utils-->>Handler: JSON { query, results: [flat dicts] }
Handler-->>CLI: Print JSON (flat)
Handler-->>MCP: JSON-RPC response (flat)
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Greptile SummaryThis PR adds a
Confidence Score: 4/5Safe to merge; the logic is correct and the new parameter is well-tested across all three states. The core truncation logic in result_to_dict has a minor inconsistency: when Some(n) is requested but the chunk content is empty, join produces an empty string and the field is emitted as content: '' rather than being omitted. The resolve_snippet_lines helper is also duplicated verbatim between the two binary modules instead of living in the shared library crate. Both are low-impact and do not affect normal use. crates/csp/src/utils.rs — the Some(n) branch in result_to_dict; crates/csp/src/bin/csp/mcp_server.rs — the duplicated resolve_snippet_lines. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
CLI["CLI: --max-snippet-lines N\nOption i64, default absent = full"]
MCP["MCP: max_snippet_lines\nabsent=Some10, null=None"]
CLI -->|resolve_snippet_lines| R1["Option usize"]
MCP -->|resolve_snippet_lines| R2["Option usize"]
R1 --> FMT["format_results"]
R2 --> FMT
FMT --> RTD["result_to_dict"]
RTD --> NONE{max_snippet_lines}
NONE -->|None| FULL["content: full chunk text"]
NONE -->|Some 0| OMIT["content field omitted"]
NONE -->|Some n| TRUNC["content: first N lines"]
FULL --> OUT["Flat JSON output\nfile_path, start_line, end_line, score, content"]
OMIT --> OUT
TRUNC --> OUT
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
CLI["CLI: --max-snippet-lines N\nOption i64, default absent = full"]
MCP["MCP: max_snippet_lines\nabsent=Some10, null=None"]
CLI -->|resolve_snippet_lines| R1["Option usize"]
MCP -->|resolve_snippet_lines| R2["Option usize"]
R1 --> FMT["format_results"]
R2 --> FMT
FMT --> RTD["result_to_dict"]
RTD --> NONE{max_snippet_lines}
NONE -->|None| FULL["content: full chunk text"]
NONE -->|Some 0| OMIT["content field omitted"]
NONE -->|Some n| TRUNC["content: first N lines"]
FULL --> OUT["Flat JSON output\nfile_path, start_line, end_line, score, content"]
OMIT --> OUT
TRUNC --> OUT
Prompt To Fix All With AIFix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
crates/csp/src/utils.rs:30-33
When `Some(n)` is requested but `content` is an empty string, `lines()` yields nothing, `take(n)` yields nothing, and `join("
")` produces `""` — so the field is emitted as `content: ""` rather than being omitted. This is inconsistent with `Some(0)` which skips the field entirely, and could confuse callers that check `result.content.is_some()` to decide whether to navigate to the file. An explicit guard keeps the three cases consistently distinct.
```suggestion
Some(n) => {
let snippet: Vec<&str> = c.content.lines().take(n).collect();
if !snippet.is_empty() {
entry["content"] = json!(snippet.join("\n"));
}
}
```
### Issue 2 of 2
crates/csp/src/bin/csp/mcp_server.rs:29-31
**Duplicated `resolve_snippet_lines` helper**
`resolve_snippet_lines` (and its `default_max_snippet_lines` companion) is defined identically in both `mcp_server.rs` and `main.rs`. Since both binaries already depend on the `csp` library crate (they import from `csp::utils`, `csp::mcp`, etc.), moving this pair to `csp::utils` (or a dedicated `csp::snippet` submodule) and re-exporting it would eliminate the duplication and keep the single source of truth close to `result_to_dict`, which it directly feeds.
Reviews (1): Last reviewed commit: "feat(search): add max_snippet_lines to c..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
5 tasks
- Move resolve_snippet_lines into csp::utils; drop the duplicate copies in the CLI and MCP server binaries (Greptile, Gemini Code Assist) - Split snippet lines with the chunker's splitlines-equivalent so bare CR breaks lines the way upstream Python splitlines() does
Contributor
Author
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a --max-snippet-lines option to the CLI and MCP server to limit the number of source lines returned per result, updating documentation, tests, and formatting logic. Feedback on the changes highlights a potential integer truncation issue on 32-bit systems when casting i64 to usize in resolve_snippet_lines, as well as a performance inefficiency where the entire chunk content is split into lines even when only a small snippet is requested.
Use usize::try_from with a usize::MAX fallback in resolve_snippet_lines so an i64 above the platform usize range saturates rather than truncating.
|
Merged
5 tasks
amondnet
added a commit
that referenced
this pull request
Sep 4, 2026
…y) into incremental reindexing
amondnet
added a commit
that referenced
this pull request
Sep 4, 2026
…vectors, and BM25 postings (#91) * feat(index): incremental reindexing — reuse unchanged files' chunks, vectors, and BM25 postings Port upstream semble #225 (partial reindexing) to the Rust core. When the cached index's whole-tree content hash is stale, `load_or_build_index` now seeds the rebuild with the previous index instead of rebuilding from scratch: files whose per-file content hash is unchanged keep their chunks, vector rows, and BM25 postings; only changed files are re-chunked and re-embedded, and deleted files' postings are dropped. - `indexing/types.rs`: `FileManifestEntry {hash, start, count}`, `PreviousIndex::try_new` (alignment checks), `make_chunk_id`. - `sparse.rs`: `Bm25Index` becomes the id-keyed incremental index from upstream `bm25.py` (`add_document` / `remove_document` / `set_doc_order`); `bm25.json` v2 persists `{documents, docOrder}`. - `create.rs`: `create_index_from_path(.., previous)` reuse path; rows are moved (not copied) and reused rows are not re-normalised. - `cache_orchestrator.rs`: `load_previous_for_incremental` (fails closed on any structural inconsistency) + shared `manifest_compatible`. - `index.rs`: `files` manifest in `IndexManifest`/`CspIndex`, `from_path_with_previous`, `INDEX_SCHEMA_VERSION` 1 → 2, and `load_from_disk` rejects component count mismatches. - ADR-0005 records the per-file content hash (vs upstream `mtime_ns`) decision; `semble.md` and both READMEs updated. Refs #84 * fix(index): harden incremental reindex after review - `PreviousIndex::try_new`: sort manifest entries by `(start, count)` so a zero-chunk file that ties with the following file no longer fails the tiling check (which silently disabled incremental reuse for that tree). Regression test `zero_chunk_file_does_not_break_manifest_tiling`. - `Bm25Index::load`: rebuild postings from the persisted term counts via `insert_document` instead of materialising `freq` copies of every term; sum lengths in u64 and reject out-of-range counts. Drop the duplicate `Doc.chunk_id`. - `create_index_from_path`: embed all changed files' chunks in one batched pass (`dense::embed_chunk_refs`) so a cold build keeps the tokenizer's batch parallelism. - `FileManifestEntry::end()`: saturating add so a corrupt manifest fails the range checks instead of overflowing. - `load_previous_for_incremental`: reject a seed whose vector rows do not match the live model's dimension, falling back to a full rebuild. - `parse_manifest`: read `files` through the `FileManifestEntry` serde derive that `save` writes with. - Docs: query-term de-duplication is a real ranking divergence from upstream's query-frequency weighting, not rank-neutral; record it as an open parity gap in ADR-0005 and `semble.md`. Refs #84 * fix(index): skip files whose lossy display path collides with an indexed file On Unix, file names that differ only in invalid UTF-8 bytes collapse to the same `to_string_lossy` path. The BM25 chunk ids derived from that path would then collide and abort the whole build with "chunk_id already indexed". Keep the first such file, skip the rest with a warning, and add a Linux-only regression test (APFS rejects non-UTF-8 names). Refs #84 * chore: merge origin/main (#80 max_snippet_lines, #82 savings telemetry) into incremental reindexing * fix(index): reject zero BM25 term counts on load; take persisted vectors verbatim - Bm25Index::load rejects a zero term frequency (it would inflate the term's document frequency) so the cache falls back to a full rebuild. - SelectableBasicBackend::load no longer re-normalises rows that were normalised before save, keeping unchanged rows bit-identical across an incremental rebuild seeded from disk. Refs #84 * refactor(index): split create/sparse tests out, extract create_index_from_path helpers - create.rs / sparse.rs test modules move to create/tests.rs and sparse/tests.rs, matching the index/, dense/, cache_orchestrator/ layout. - create_index_from_path delegates to open_previous, display_path, take_previous_rows and embed_fresh_rows; behaviour unchanged. - load_previous_for_incremental compares the content selection as a set, so a duplicated request no longer matches a manifest that covers more. Refs #84 * perf(index): compare the cached backend dim instead of scanning every row Refs #84 * test(index): build the manifest key with the platform separator Refs #84
2 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Addresses #75. Ports upstream semble#198 (
d561953).What & why
semble#198's insight: a search snippet is a locator, not the final content — the agent usually only needs enough to confirm it found the right place, then navigates to the file. Returning full chunks by default wastes tokens.
max_snippet_lineslets results carry a short preview instead.Semantics (
utils::format_results/result_to_dict):None→ full chunk content0→ omitcontent(file path + line range only)N > 0→ firstNlinesSurface:
search/find-related--max-snippet-lines Nsearch/find_relatedmax_snippet_linesMCP default is tri-state via a serde field default: field absent → 10; JSON
null→ full chunk;0→ location only.Wire-shape flatten (faithful to upstream)
Per discussion, this follows upstream faithfully. #198 also flattened the wire dict, so results are now:
{ "file_path": "...", "start_line": 1, "end_line": 9, "score": 0.87, "content": "..." }top-level, dropping the nested
chunkwrapper,location, andlanguage. Context: csp's previous nested shape was itself a faithful mirror of upstream's pre-#198SearchResult.to_dict— upstream reshaped it in #198, so matching it keeps parity. The librarySearchResultstruct is unchanged; only the CLI/MCP JSON envelope changed.Out of scope
save_search_statsinto the search flow at all (CspIndexhas nofile_sizes), so savings telemetry isn't recorded yet and there's nothing for #206 to correct. Wiring savings is a separate pre-existing gap; #206 rides on it. Recommend a dedicated issue.--agent/flags line — deliberately untouched to avoid a merge conflict with docs: correct thecsp --agentlist in CLAUDE.md #79 (which already rewrites that exact line). The--max-snippet-linesflag should be added to that Public API bullet once docs: correct thecsp --agentlist in CLAUDE.md #79 lands.Verification
cargo fmt --all✅ ·cargo clippy --all-targets --all-features -- -D warnings✅cargo test --workspace✅ — 270 lib + 21 CLI, incl. new unit tests for None/N/0 truncation (utils), CLIsearch_output_caps_snippet_lines, MCPsearch_tool_respects_max_snippet_lines_zero, and the tri-state param default (mcp_server).chunk/location);--max-snippet-lines 2→ first 2 lines;--max-snippet-lines 0→ nocontent, location kept.Summary by cubic
Adds a
max_snippet_linescap to search results so they return a short preview instead of full chunks, reducing token usage for agents. Also flattens the results JSON to top-level fields for parity with upstream.New Features
--max-snippet-lines Nonsearchandfind-related. None → full chunk (default), 0 → nocontent, N>0 → first N lines. Negative values clamp to 0.max_snippet_linesparam onsearchandfind_related. Default is 10. Passnullfor full chunk, or0for location-only.Migration
{ file_path, start_line, end_line, score, content? }. The nestedchunk/location/languagefields were removed; update any parsers.null(MCP) or omit the flag (CLI) for full chunks, or0for location-only.Written for commit c59f471. Summary will update on new commits.